feat: Implement folder batch import and filmstrip UI#63
Open
amemya wants to merge 1 commit into
Open
Conversation
- Unified 'Open Photos' and 'Open Folder' into a single OS dialog allowing both file and directory selection
- Implemented filmstrip UI for batch image navigation
- Extracted 'ensureValidExtension' helper in Go backend to safely validate and append extensions for batch exports
- Fixed critical UI freeze bug caused by 'isSelectingRef' leaking on dialog cancellation
- Corrected 'ExifResult' TypeScript interface to strictly match the flat JSON structure emitted by the Go backend
- Implemented robust error skipping for 'filepath.Walk' to prevent a single unreadable file from aborting the entire import process
- Cleaned up unused state ('openMenuVisible') and resolved variable shadowing in map callbacks
- Removed deprecated click-to-open logic from the empty state in favor of clear action buttons
Contributor
📝 WalkthroughSummary by CodeRabbit新機能
ウォークスルーバックエンド API を拡張し、複数ファイル・フォルダの一括選択と EXIF 抽出に対応。フロントエンド状態を単一画像から複数画像配列に再構成し、個別選択・EXIF 編集・一括適用・バッチエクスポートのフロー を実装。UI とスタイルをマルチイメージ対応に再設計。 変更内容マルチイメージとフォルダ対応
推定レビュー工数🎯 4 (Complex) | ⏱️ ~60 minutes 関連する可能性のある PR
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app.go`:
- Around line 155-203: ProcessPaths collects duplicate file paths into
validPaths causing duplicate processing; fix by normalizing each candidate path
(use filepath.Clean and preferably filepath.Abs) and deduplicating before
calling ProcessImageFile: maintain a map[string]struct{} seen and when adding to
validPaths check seen[normalized]==false then mark seen[normalized]=struct{}{}
and append; you can apply this either when you append inside the Walk callback
and the else branch (replace direct validPaths.append with the normalized+seen
check) or run a short dedupe pass over validPaths before the final loop; keep
using a.ProcessImageFile(path) afterward with the normalized path.
- Around line 490-507: SaveBatchImage currently builds savePath with
filepath.Join(exportDir, exportName) without validating the target is inside
exportDir; fix SaveBatchImage to (1) reject empty exportDir, (2) reject
exportName if filepath.IsAbs(exportName) or if filepath.Clean(exportName)
contains ".." path elements, (3) construct candidate := filepath.Join(exportDir,
exportName), resolve abs paths with filepath.Abs and filepath.Clean for both
exportDir and candidate, then use filepath.Rel(exportDirAbs, candidateAbs) and
ensure the relative path does not start with ".." (or return a SaveResult
error), and only then call ensureValidExtension(candidate, isPng) and proceed to
prepareSave; update references to savePath in the function accordingly so no
unvalidated path reaches handler.prepareSave.
In `@frontend/src/App.tsx`:
- Around line 703-709: The code always calls showToast("Export complete!") even
when the HTTP POST failed; update the save/export flow (the block that checks
response.ok in the try/catch where response and exportName are used) to only
show the success toast on actual success—either add a return after handling the
error inside the if (!response.ok) branch or move the showToast call into the
success path after the response.ok check so that failed POSTs do not display
"Export complete!".
- Around line 947-953: The filmstrip item uses a non-focusable <div> with
onClick (className "filmstrip-item", selectedIndex and setSelectedIndex) which
prevents keyboard selection; change the element to a semantic <button> (or if
you must keep a div: add role="button", tabIndex={0} and keydown handler for
Enter/Space that calls setSelectedIndex(idx)) so it can receive focus and
activate via keyboard, preserve the existing id (`thumb-${idx}`), img src
(img.imageURL), alt text, loading and draggable props, and ensure the selected
styling still toggles based on selectedIndex.
- Around line 390-410: The img.onload / img.onerror handlers currently spread
the stale captured variable current into the updated item, which can overwrite
newer EXIF edits made via setExif; change the setImportedImages updater in both
img.onload and img.onerror to base the replacement on the existing prev entry
(use prev[idx] or prev[selectedIndex] as the source) and only overwrite the
specific fields (imageObj or loadError) so concurrent edits to other fields are
preserved; keep the same index-finding logic and only mutate those fields when
idx !== -1.
- Around line 770-809: The current batch export loop lets exceptions from
AppAPI.SaveBatchImage, offCanvas.toBlob, or the fetch call bubble up and abort
the entire operation; fix this by wrapping the per-image processing logic (the
code that calls AppAPI.SaveBatchImage, creates the Blob via offCanvas.toBlob,
and posts it with fetch) in its own try/catch so any error increments failCount
and continues to the next image, and ensure caught errors are logged
(console.error) and do not rethrow so the outer try only handles truly fatal
errors; adjust successCount/failCount updates accordingly and keep the existing
overall success/failure showToast behavior.
- Around line 430-451: When replacing importedImages you must revoke previous
blob URLs to avoid memory leaks: in the setImportedImages updater (the callback
passed to setImportedImages) iterate over the previous state and call
URL.revokeObjectURL(img.imageURL) for each imageURL that is a blob URL before
returning the new ImportedImage[] (check for img.imageURL &&
img.imageURL.startsWith('blob:') to be safe); additionally add a useEffect
cleanup that runs on unmount to revoke any remaining importedImages' imageURL
values (again checking for blob:), and keep using URL.revokeObjectURL when you
remove/replace individual ImportedImage entries elsewhere; reference the
setImportedImages updater, ImportedImage.imageURL, and add a component-level
useEffect cleanup to complete the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c93ecb16-0067-4fa9-b219-998592089ad1
📒 Files selected for processing (5)
app.gofrontend/src/App.cssfrontend/src/App.tsxfrontend/src/components/MetadataSettingsPanel.tsxfrontend/src/types.ts
Comment on lines
+155
to
+203
| // ProcessPaths recursively walks provided paths (or single files) and processes valid images. | ||
| func (a *App) ProcessPaths(paths []string) []ExifResult { | ||
| var results []ExifResult | ||
| var validPaths []string | ||
|
|
||
| for _, p := range paths { | ||
| info, err := os.Stat(p) | ||
| if err != nil { | ||
| results = append(results, ExifResult{Error: "Failed to access path: " + err.Error(), FilePath: p}) | ||
| continue | ||
| } | ||
|
|
||
| if info.IsDir() { | ||
| err = filepath.Walk(p, func(path string, info os.FileInfo, err error) error { | ||
| if err != nil { | ||
| log.Printf("Error accessing path %s: %v", path, err) | ||
| return nil // Skip this file/folder but continue walking | ||
| } | ||
| if !info.IsDir() { | ||
| lower := strings.ToLower(path) | ||
| if strings.HasSuffix(lower, ".jpg") || strings.HasSuffix(lower, ".jpeg") || strings.HasSuffix(lower, ".png") { | ||
| validPaths = append(validPaths, path) | ||
| } | ||
| } | ||
| return nil | ||
| }) | ||
| if err != nil { | ||
| results = append(results, ExifResult{Error: "Failed to read directory: " + err.Error(), FilePath: p}) | ||
| } | ||
| } else { | ||
| validPaths = append(validPaths, p) | ||
| } | ||
| } | ||
|
|
||
| for _, path := range validPaths { | ||
| res := a.ProcessImageFile(path) | ||
| if res.Error != "" { | ||
| log.Printf("Skipped file %s: %v", path, res.Error) | ||
| continue | ||
| } | ||
| results = append(results, res) | ||
| } | ||
|
|
||
| if len(results) == 0 { | ||
| return []ExifResult{{Error: "No valid images found in the selected paths."}} | ||
| } | ||
|
|
||
| return results | ||
| } |
Contributor
There was a problem hiding this comment.
重複選択時に同一画像が二重取り込みされます。
Line 160-186 では validPaths の重複排除がないため、フォルダとその配下ファイルを同時選択したケースで同じパスが複数回 ProcessImageFile されます。filmstrip と一括エクスポート結果が重複します。
🔧 修正案（重複パス除去）
func (a *App) ProcessPaths(paths []string) []ExifResult {
var results []ExifResult
var validPaths []string
+ seen := make(map[string]struct{})
+ addValidPath := func(p string) {
+ clean := filepath.Clean(p)
+ if _, ok := seen[clean]; ok {
+ return
+ }
+ seen[clean] = struct{}{}
+ validPaths = append(validPaths, clean)
+ }
for _, p := range paths {
info, err := os.Stat(p)
@@
if !info.IsDir() {
lower := strings.ToLower(path)
if strings.HasSuffix(lower, ".jpg") || strings.HasSuffix(lower, ".jpeg") || strings.HasSuffix(lower, ".png") {
- validPaths = append(validPaths, path)
+ addValidPath(path)
}
}
return nil
@@
} else {
- validPaths = append(validPaths, p)
+ addValidPath(p)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // ProcessPaths recursively walks provided paths (or single files) and processes valid images. | |
| func (a *App) ProcessPaths(paths []string) []ExifResult { | |
| var results []ExifResult | |
| var validPaths []string | |
| for _, p := range paths { | |
| info, err := os.Stat(p) | |
| if err != nil { | |
| results = append(results, ExifResult{Error: "Failed to access path: " + err.Error(), FilePath: p}) | |
| continue | |
| } | |
| if info.IsDir() { | |
| err = filepath.Walk(p, func(path string, info os.FileInfo, err error) error { | |
| if err != nil { | |
| log.Printf("Error accessing path %s: %v", path, err) | |
| return nil // Skip this file/folder but continue walking | |
| } | |
| if !info.IsDir() { | |
| lower := strings.ToLower(path) | |
| if strings.HasSuffix(lower, ".jpg") || strings.HasSuffix(lower, ".jpeg") || strings.HasSuffix(lower, ".png") { | |
| validPaths = append(validPaths, path) | |
| } | |
| } | |
| return nil | |
| }) | |
| if err != nil { | |
| results = append(results, ExifResult{Error: "Failed to read directory: " + err.Error(), FilePath: p}) | |
| } | |
| } else { | |
| validPaths = append(validPaths, p) | |
| } | |
| } | |
| for _, path := range validPaths { | |
| res := a.ProcessImageFile(path) | |
| if res.Error != "" { | |
| log.Printf("Skipped file %s: %v", path, res.Error) | |
| continue | |
| } | |
| results = append(results, res) | |
| } | |
| if len(results) == 0 { | |
| return []ExifResult{{Error: "No valid images found in the selected paths."}} | |
| } | |
| return results | |
| } | |
| // ProcessPaths recursively walks provided paths (or single files) and processes valid images. | |
| func (a *App) ProcessPaths(paths []string) []ExifResult { | |
| var results []ExifResult | |
| var validPaths []string | |
| seen := make(map[string]struct{}) | |
| addValidPath := func(p string) { | |
| clean := filepath.Clean(p) | |
| if _, ok := seen[clean]; ok { | |
| return | |
| } | |
| seen[clean] = struct{}{} | |
| validPaths = append(validPaths, clean) | |
| } | |
| for _, p := range paths { | |
| info, err := os.Stat(p) | |
| if err != nil { | |
| results = append(results, ExifResult{Error: "Failed to access path: " + err.Error(), FilePath: p}) | |
| continue | |
| } | |
| if info.IsDir() { | |
| err = filepath.Walk(p, func(path string, info os.FileInfo, err error) error { | |
| if err != nil { | |
| log.Printf("Error accessing path %s: %v", path, err) | |
| return nil // Skip this file/folder but continue walking | |
| } | |
| if !info.IsDir() { | |
| lower := strings.ToLower(path) | |
| if strings.HasSuffix(lower, ".jpg") || strings.HasSuffix(lower, ".jpeg") || strings.HasSuffix(lower, ".png") { | |
| addValidPath(path) | |
| } | |
| } | |
| return nil | |
| }) | |
| if err != nil { | |
| results = append(results, ExifResult{Error: "Failed to read directory: " + err.Error(), FilePath: p}) | |
| } | |
| } else { | |
| addValidPath(p) | |
| } | |
| } | |
| for _, path := range validPaths { | |
| res := a.ProcessImageFile(path) | |
| if res.Error != "" { | |
| log.Printf("Skipped file %s: %v", path, res.Error) | |
| continue | |
| } | |
| results = append(results, res) | |
| } | |
| if len(results) == 0 { | |
| return []ExifResult{{Error: "No valid images found in the selected paths."}} | |
| } | |
| return results | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app.go` around lines 155 - 203, ProcessPaths collects duplicate file paths
into validPaths causing duplicate processing; fix by normalizing each candidate
path (use filepath.Clean and preferably filepath.Abs) and deduplicating before
calling ProcessImageFile: maintain a map[string]struct{} seen and when adding to
validPaths check seen[normalized]==false then mark seen[normalized]=struct{}{}
and append; you can apply this either when you append inside the Walk callback
and the else branch (replace direct validPaths.append with the normalized+seen
check) or run a short dedupe pass over validPaths before the final loop; keep
using a.ProcessImageFile(path) afterward with the normalized path.
Comment on lines
+490
to
+507
| // SaveBatchImage bypasses ExportFolder validation for explicit batch exports. | ||
| func (a *App) SaveBatchImage(isPng bool, exportDir string, exportName string) SaveResult { | ||
| savePath := filepath.Join(exportDir, exportName) | ||
| savePath, err := ensureValidExtension(savePath, isPng) | ||
| if err != nil { | ||
| return SaveResult{Error: err.Error()} | ||
| } | ||
|
|
||
| expectedMime := "image/jpeg" | ||
| if isPng { | ||
| expectedMime = "image/png" | ||
| } | ||
| if a.handler == nil { | ||
| return SaveResult{Error: "Internal error: image handler not initialized"} | ||
| } | ||
| token := a.handler.prepareSave(savePath, expectedMime) | ||
| return SaveResult{SaveToken: token} | ||
| } |
Contributor
There was a problem hiding this comment.
SaveBatchImage が保存先ディレクトリ境界を検証していません。
Line 492-493 は filepath.Join(exportDir, exportName) を未検証で使っており、exportName に ../ や絶対パスが入ると exportDir 外へ保存可能です。さらに exportDir が空の場合、相対パス保存になります。バッチ保存API側で境界検証を入れてください。
🛡️ 修正案（空値チェック + ディレクトリ外書き込み防止）
func (a *App) SaveBatchImage(isPng bool, exportDir string, exportName string) SaveResult {
- savePath := filepath.Join(exportDir, exportName)
- savePath, err := ensureValidExtension(savePath, isPng)
+ if strings.TrimSpace(exportDir) == "" {
+ return SaveResult{Error: "Export directory is required"}
+ }
+ if strings.TrimSpace(exportName) == "" {
+ return SaveResult{Error: "Export filename is required"}
+ }
+
+ baseDir := filepath.Clean(exportDir)
+ candidate := filepath.Join(baseDir, exportName)
+ rel, err := filepath.Rel(baseDir, candidate)
+ if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
+ return SaveResult{Error: "Invalid export filename"}
+ }
+
+ savePath, err := ensureValidExtension(candidate, isPng)
if err != nil {
return SaveResult{Error: err.Error()}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // SaveBatchImage bypasses ExportFolder validation for explicit batch exports. | |
| func (a *App) SaveBatchImage(isPng bool, exportDir string, exportName string) SaveResult { | |
| savePath := filepath.Join(exportDir, exportName) | |
| savePath, err := ensureValidExtension(savePath, isPng) | |
| if err != nil { | |
| return SaveResult{Error: err.Error()} | |
| } | |
| expectedMime := "image/jpeg" | |
| if isPng { | |
| expectedMime = "image/png" | |
| } | |
| if a.handler == nil { | |
| return SaveResult{Error: "Internal error: image handler not initialized"} | |
| } | |
| token := a.handler.prepareSave(savePath, expectedMime) | |
| return SaveResult{SaveToken: token} | |
| } | |
| // SaveBatchImage bypasses ExportFolder validation for explicit batch exports. | |
| func (a *App) SaveBatchImage(isPng bool, exportDir string, exportName string) SaveResult { | |
| if strings.TrimSpace(exportDir) == "" { | |
| return SaveResult{Error: "Export directory is required"} | |
| } | |
| if strings.TrimSpace(exportName) == "" { | |
| return SaveResult{Error: "Export filename is required"} | |
| } | |
| baseDir := filepath.Clean(exportDir) | |
| candidate := filepath.Join(baseDir, exportName) | |
| rel, err := filepath.Rel(baseDir, candidate) | |
| if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { | |
| return SaveResult{Error: "Invalid export filename"} | |
| } | |
| savePath, err := ensureValidExtension(candidate, isPng) | |
| if err != nil { | |
| return SaveResult{Error: err.Error()} | |
| } | |
| expectedMime := "image/jpeg" | |
| if isPng { | |
| expectedMime = "image/png" | |
| } | |
| if a.handler == nil { | |
| return SaveResult{Error: "Internal error: image handler not initialized"} | |
| } | |
| token := a.handler.prepareSave(savePath, expectedMime) | |
| return SaveResult{SaveToken: token} | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app.go` around lines 490 - 507, SaveBatchImage currently builds savePath with
filepath.Join(exportDir, exportName) without validating the target is inside
exportDir; fix SaveBatchImage to (1) reject empty exportDir, (2) reject
exportName if filepath.IsAbs(exportName) or if filepath.Clean(exportName)
contains ".." path elements, (3) construct candidate := filepath.Join(exportDir,
exportName), resolve abs paths with filepath.Abs and filepath.Clean for both
exportDir and candidate, then use filepath.Rel(exportDirAbs, candidateAbs) and
ensure the relative path does not start with ".." (or return a SaveResult
error), and only then call ensureValidExtension(candidate, isPng) and proceed to
prepareSave; update references to savePath in the function accordingly so no
unvalidated path reaches handler.prepareSave.
Comment on lines
+390
to
+410
| setImportedImages(prev => { | ||
| const newImages = [...prev]; | ||
| // Make sure the image we loaded is still at this index (or find by url if order changed, but we only append for now) | ||
| if (newImages[selectedIndex] && newImages[selectedIndex].imageURL === current.imageURL) { | ||
| newImages[selectedIndex] = { ...current, imageObj: img }; | ||
| } else { | ||
| // Fallback: find it | ||
| const idx = newImages.findIndex(item => item.imageURL === current.imageURL); | ||
| if (idx !== -1) newImages[idx] = { ...current, imageObj: img }; | ||
| } | ||
| return newImages; | ||
| }); | ||
| setOrientation(img.height > img.width ? "portrait" : "landscape"); | ||
| setImageLoaded(true); | ||
| resolve(); | ||
| }; | ||
| img.onerror = () => { | ||
| console.error("Failed to load image"); | ||
| showToast("Failed to load image preview"); | ||
| reject(new Error("Failed to load image")); | ||
| setImportedImages(prev => { | ||
| const newImages = [...prev]; | ||
| const idx = newImages.findIndex(item => item.imageURL === current.imageURL); | ||
| if (idx !== -1) newImages[idx] = { ...current, loadError: true }; | ||
| return newImages; |
Contributor
There was a problem hiding this comment.
非同期ロード完了時に古い current を再代入して、直近のEXIF編集を巻き戻す可能性があります。
img.onload / img.onerror で { ...current, ... } を使っているため、ロード中に行われた setExif 更新が失われます。prev[idx] をベースに imageObj / loadError だけ更新してください。
💡 修正案
img.onload = () => {
setImportedImages(prev => {
const newImages = [...prev];
- if (newImages[selectedIndex] && newImages[selectedIndex].imageURL === current.imageURL) {
- newImages[selectedIndex] = { ...current, imageObj: img };
- } else {
- const idx = newImages.findIndex(item => item.imageURL === current.imageURL);
- if (idx !== -1) newImages[idx] = { ...current, imageObj: img };
- }
+ const idx = newImages.findIndex(item => item.imageURL === current.imageURL);
+ if (idx !== -1) {
+ newImages[idx] = { ...newImages[idx], imageObj: img, loadError: false };
+ }
return newImages;
});
};
img.onerror = () => {
showToast("Failed to load image preview");
setImportedImages(prev => {
const newImages = [...prev];
const idx = newImages.findIndex(item => item.imageURL === current.imageURL);
- if (idx !== -1) newImages[idx] = { ...current, loadError: true };
+ if (idx !== -1) newImages[idx] = { ...newImages[idx], loadError: true };
return newImages;
});
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/App.tsx` around lines 390 - 410, The img.onload / img.onerror
handlers currently spread the stale captured variable current into the updated
item, which can overwrite newer EXIF edits made via setExif; change the
setImportedImages updater in both img.onload and img.onerror to base the
replacement on the existing prev entry (use prev[idx] or prev[selectedIndex] as
the source) and only overwrite the specific fields (imageObj or loadError) so
concurrent edits to other fields are preserved; keep the same index-finding
logic and only mutate those fields when idx !== -1.
Comment on lines
+430
to
451
| setImportedImages(() => { | ||
| const newImages: ImportedImage[] = validResults.map(r => ({ | ||
| filePath: r.filePath || "", | ||
| imageURL: r.imageURL!, | ||
| sourceMimeType: (r.mimeType as 'image/jpeg' | 'image/png') || 'image/jpeg', | ||
| imageObj: null, | ||
| exif: { | ||
| camera: r.camera || "", | ||
| lens: r.lens || "", | ||
| focalLength: r.focalLength || "", | ||
| aperture: r.aperture || "", | ||
| shutterSpeed: r.shutterSpeed || "", | ||
| iso: r.iso || "", | ||
| film: "", | ||
| developer: "", | ||
| dilution: "", | ||
| temperature: "", | ||
| time: "", | ||
| } | ||
| })); | ||
| return newImages; | ||
| }); |
Contributor
There was a problem hiding this comment.
importedImages を置き換えるたびに旧 imageURL の解放がなく、バッチ利用でメモリが増加し続けます。
ImportedImage.imageURL は Blob URL 前提なので、差し替え時とアンマウント時に URL.revokeObjectURL で解放してください。
💡 修正案
- setImportedImages(() => {
+ setImportedImages(prev => {
+ for (const oldImg of prev) {
+ if (oldImg.imageURL.startsWith('blob:')) {
+ URL.revokeObjectURL(oldImg.imageURL);
+ }
+ }
const newImages: ImportedImage[] = validResults.map(r => ({
filePath: r.filePath || "",
imageURL: r.imageURL!,
sourceMimeType: (r.mimeType as 'image/jpeg' | 'image/png') || 'image/jpeg',
imageObj: null,
exif: {
camera: r.camera || "",
lens: r.lens || "",
focalLength: r.focalLength || "",
aperture: r.aperture || "",
shutterSpeed: r.shutterSpeed || "",
iso: r.iso || "",
film: "",
developer: "",
dilution: "",
temperature: "",
time: "",
}
}));
return newImages;
});+ useEffect(() => {
+ return () => {
+ for (const img of importedImages) {
+ if (img.imageURL.startsWith('blob:')) {
+ URL.revokeObjectURL(img.imageURL);
+ }
+ }
+ };
+ }, [importedImages]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| setImportedImages(() => { | |
| const newImages: ImportedImage[] = validResults.map(r => ({ | |
| filePath: r.filePath || "", | |
| imageURL: r.imageURL!, | |
| sourceMimeType: (r.mimeType as 'image/jpeg' | 'image/png') || 'image/jpeg', | |
| imageObj: null, | |
| exif: { | |
| camera: r.camera || "", | |
| lens: r.lens || "", | |
| focalLength: r.focalLength || "", | |
| aperture: r.aperture || "", | |
| shutterSpeed: r.shutterSpeed || "", | |
| iso: r.iso || "", | |
| film: "", | |
| developer: "", | |
| dilution: "", | |
| temperature: "", | |
| time: "", | |
| } | |
| })); | |
| return newImages; | |
| }); | |
| setImportedImages(prev => { | |
| for (const oldImg of prev) { | |
| if (oldImg.imageURL.startsWith('blob:')) { | |
| URL.revokeObjectURL(oldImg.imageURL); | |
| } | |
| } | |
| const newImages: ImportedImage[] = validResults.map(r => ({ | |
| filePath: r.filePath || "", | |
| imageURL: r.imageURL!, | |
| sourceMimeType: (r.mimeType as 'image/jpeg' | 'image/png') || 'image/jpeg', | |
| imageObj: null, | |
| exif: { | |
| camera: r.camera || "", | |
| lens: r.lens || "", | |
| focalLength: r.focalLength || "", | |
| aperture: r.aperture || "", | |
| shutterSpeed: r.shutterSpeed || "", | |
| iso: r.iso || "", | |
| film: "", | |
| developer: "", | |
| dilution: "", | |
| temperature: "", | |
| time: "", | |
| } | |
| })); | |
| return newImages; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/App.tsx` around lines 430 - 451, When replacing importedImages
you must revoke previous blob URLs to avoid memory leaks: in the
setImportedImages updater (the callback passed to setImportedImages) iterate
over the previous state and call URL.revokeObjectURL(img.imageURL) for each
imageURL that is a blob URL before returning the new ImportedImage[] (check for
img.imageURL && img.imageURL.startsWith('blob:') to be safe); additionally add a
useEffect cleanup that runs on unmount to revoke any remaining importedImages'
imageURL values (again checking for blob:), and keep using URL.revokeObjectURL
when you remove/replace individual ImportedImage entries elsewhere; reference
the setImportedImages updater, ImportedImage.imageURL, and add a component-level
useEffect cleanup to complete the fix.
Comment on lines
703
to
709
| if (!response.ok) { | ||
| const errText = await response.text(); | ||
| console.error("Save failed:", errText); | ||
| alert("Failed to save image: " + errText); | ||
| } else { | ||
| showToast("Image saved successfully"); | ||
| const text = await response.text(); | ||
| console.error("HTTP POST failed for", exportName, text); | ||
| alert("Failed to save image via HTTP POST: " + text); | ||
| } | ||
| showToast("Export complete!"); | ||
| } catch (err) { |
Contributor
There was a problem hiding this comment.
保存失敗時でも「Export complete!」トーストが必ず表示されています。
HTTP POST 失敗分岐で return するか、成功時のみトースト表示にしてください。
💡 修正案
if (!response.ok) {
const text = await response.text();
console.error("HTTP POST failed for", exportName, text);
alert("Failed to save image via HTTP POST: " + text);
+ return;
}
showToast("Export complete!");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!response.ok) { | |
| const errText = await response.text(); | |
| console.error("Save failed:", errText); | |
| alert("Failed to save image: " + errText); | |
| } else { | |
| showToast("Image saved successfully"); | |
| const text = await response.text(); | |
| console.error("HTTP POST failed for", exportName, text); | |
| alert("Failed to save image via HTTP POST: " + text); | |
| } | |
| showToast("Export complete!"); | |
| } catch (err) { | |
| if (!response.ok) { | |
| const text = await response.text(); | |
| console.error("HTTP POST failed for", exportName, text); | |
| alert("Failed to save image via HTTP POST: " + text); | |
| return; | |
| } | |
| showToast("Export complete!"); | |
| } catch (err) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/App.tsx` around lines 703 - 709, The code always calls
showToast("Export complete!") even when the HTTP POST failed; update the
save/export flow (the block that checks response.ok in the try/catch where
response and exportName are used) to only show the success toast on actual
success—either add a return after handling the error inside the if
(!response.ok) branch or move the showToast call into the success path after the
response.ok check so that failed POSTs do not display "Export complete!".
Comment on lines
+770
to
+809
| const result = await AppAPI.SaveBatchImage(isPng, exportDir, exportName); | ||
| if (result.error) { | ||
| console.error("Export failed for", exportName, result.error); | ||
| failCount++; | ||
| continue; | ||
| } | ||
|
|
||
| const blob = await new Promise<Blob>((resolve, reject) => { | ||
| offCanvas.toBlob( | ||
| (b) => b ? resolve(b) : reject(new Error("toBlob returned null")), | ||
| targetMime, | ||
| 1.0 | ||
| ); | ||
| }); | ||
|
|
||
| const response = await fetch(`/api/save?token=${result.saveToken}`, { | ||
| method: 'POST', | ||
| body: blob, | ||
| headers: { 'Content-Type': targetMime } | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| const text = await response.text(); | ||
| console.error("Save failed:", text); | ||
| failCount++; | ||
| continue; | ||
| } | ||
|
|
||
| successCount++; | ||
| } | ||
|
|
||
| if (failCount > 0) { | ||
| showToast(`Export complete: ${successCount} succeeded, ${failCount} failed.`); | ||
| } else { | ||
| showToast(`Successfully exported all ${successCount} images.`); | ||
| } | ||
| } catch (err: any) { | ||
| console.error("Failed to export all:", err); | ||
| showToast("Failed to export all: " + err.message); | ||
| } finally { |
Contributor
There was a problem hiding this comment.
バッチエクスポートが1件の例外で全体中断します。
ループ内の SaveBatchImage / toBlob / fetch の例外が外側 catch に伝播すると、残り画像の処理が止まります。各画像ごとに try/catch して failCount++ で継続してください。
💡 修正案
for (let i = 0; i < importedImages.length; i++) {
- const imgState = importedImages[i];
- let imgToDraw = imgState.imageObj;
- ...
- const result = await AppAPI.SaveBatchImage(isPng, exportDir, exportName);
- ...
- const blob = await new Promise<Blob>((resolve, reject) => { ... });
- const response = await fetch(`/api/save?token=${result.saveToken}`, { ... });
- if (!response.ok) { ... }
- successCount++;
+ try {
+ const imgState = importedImages[i];
+ let imgToDraw = imgState.imageObj;
+ ...
+ const result = await AppAPI.SaveBatchImage(isPng, exportDir, exportName);
+ ...
+ const blob = await new Promise<Blob>((resolve, reject) => { ... });
+ const response = await fetch(`/api/save?token=${result.saveToken}`, { ... });
+ if (!response.ok) {
+ const text = await response.text();
+ console.error("Save failed:", text);
+ failCount++;
+ continue;
+ }
+ successCount++;
+ } catch (e) {
+ console.error("Failed to export image:", e);
+ failCount++;
+ continue;
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/App.tsx` around lines 770 - 809, The current batch export loop
lets exceptions from AppAPI.SaveBatchImage, offCanvas.toBlob, or the fetch call
bubble up and abort the entire operation; fix this by wrapping the per-image
processing logic (the code that calls AppAPI.SaveBatchImage, creates the Blob
via offCanvas.toBlob, and posts it with fetch) in its own try/catch so any error
increments failCount and continues to the next image, and ensure caught errors
are logged (console.error) and do not rethrow so the outer try only handles
truly fatal errors; adjust successCount/failCount updates accordingly and keep
the existing overall success/failure showToast behavior.
Comment on lines
+947
to
+953
| <div | ||
| key={idx} | ||
| className={`filmstrip-item ${selectedIndex === idx ? 'selected' : ''}`} | ||
| onClick={() => setSelectedIndex(idx)} | ||
| > | ||
| <img id={`thumb-${idx}`} src={img.imageURL} alt={`Thumbnail ${idx}`} loading="lazy" draggable={false} /> | ||
| </div> |
Contributor
There was a problem hiding this comment.
フィルムストリップ項目がキーボード操作不可で、画像選択のアクセシビリティ要件を満たしていません。
div のクリック実装だとフォーカス移動・Enter/Space 操作ができません。button 要素化（または role="button" + tabIndex + キー処理）を入れてください。
💡 修正案
- <div
+ <button
+ type="button"
key={idx}
className={`filmstrip-item ${selectedIndex === idx ? 'selected' : ''}`}
onClick={() => setSelectedIndex(idx)}
+ aria-label={`画像 ${idx + 1} を選択`}
+ aria-pressed={selectedIndex === idx}
>
<img id={`thumb-${idx}`} src={img.imageURL} alt={`Thumbnail ${idx}`} loading="lazy" draggable={false} />
- </div>
+ </button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/App.tsx` around lines 947 - 953, The filmstrip item uses a
non-focusable <div> with onClick (className "filmstrip-item", selectedIndex and
setSelectedIndex) which prevents keyboard selection; change the element to a
semantic <button> (or if you must keep a div: add role="button", tabIndex={0}
and keydown handler for Enter/Space that calls setSelectedIndex(idx)) so it can
receive focus and activate via keyboard, preserve the existing id
(`thumb-${idx}`), img src (img.imageURL), alt text, loading and draggable props,
and ensure the selected styling still toggles based on selectedIndex.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
resolved #51